feat(suite-native): add trading slice to mobile #17120
Conversation
vytick
added
mobile
WalkthroughThe pull request introduces several updates to trading-related modules. In the 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-trading/src/tradingSlice.ts (3)
13-25: Consider adding JSDoc comments for type definitions.The type definitions are well-structured, but adding JSDoc comments would improve code documentation and IDE support.
Add documentation for the interfaces and types:
+/** + * Extends CommonTradingBuyState to include mobile-specific trading functionality + */ export interface TradingBuyState extends CommonTradingBuyState { selectedReceiveAccount: ReceiveAccount | undefined; } +/** + * Represents the complete trading state including buy operations + */ interface TradingState extends CommonTradingState { buy: TradingBuyState; } +/** + * Represents the root state structure for trading functionality + */ export type TradeRootState = { wallet: { trading: TradingState; }; };
32-51: Consider adding error handling for invalid receive accounts.The reducer should validate the receive account before setting it in the state.
Add validation in the reducer:
setBuySelectedReceiveAccount: ( state, { payload }: PayloadAction<{ selectedReceiveAccount: ReceiveAccount }>, ) => { + // Validate receive account before setting + if (!payload.selectedReceiveAccount?.address) { + console.warn('Invalid receive account provided'); + return; + } state.buy.selectedReceiveAccount = payload.selectedReceiveAccount; },
55-60: Consider memoizing complex selectors.While the current selectors are simple, consider using
createSelectorfor any future complex selectors that compute derived data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
suite-common/trading/src/index.ts(1 hunks)suite-native/module-trading/src/index.ts(1 hunks)suite-native/module-trading/src/tradingSlice.ts(1 hunks)suite-native/state/src/reducers.ts(3 hunks)suite-native/state/src/store.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: e2e-test-suite-web (@group_wallet, trezor-user-env-unix bitcoin-regtest, 1)
- GitHub Check: e2e-test-suite-web (@group_device-management, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group_suite, trezor-user-env-unix, 1)
- GitHub Check: e2e-test-suite-web (@group=other, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata2, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=metadata1, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=settings, trezor-user-env-unix)
- GitHub Check: e2e-test-suite-web (@group=suite, trezor-user-env-unix)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (7)
suite-native/module-trading/src/index.ts (1)
2-2: LGTM!The export makes all trading slice entities available to other modules, which aligns with the PR objective of adding trading functionality to mobile.
suite-common/trading/src/index.ts (1)
7-7: LGTM!The export of
TradingBuyStatetype from the buy reducer aligns with the trading slice implementation.suite-native/module-trading/src/tradingSlice.ts (1)
27-30: LGTM!The initial state correctly extends the common initial state with mobile-specific trading functionality.
suite-native/state/src/store.ts (1)
29-29: LGTM!The trading middleware is correctly integrated into the Redux store configuration.
suite-native/state/src/reducers.ts (3)
32-32: LGTM!The import statement follows the established pattern and is correctly placed among other feature module imports.
87-87: LGTM!The trading reducer is correctly integrated into the wallet reducers, making the trading state accessible under the 'wallet.trading' path.
63-63:❓ Verification inconclusive
Verify persistence requirements for trading state.
The trading reducer is correctly prepared, but unlike some other reducers (e.g., accounts, transactions), it's not configured for persistence. Please confirm if this is intentional.
🏁 Script executed:
#!/bin/bash # Description: Check if other trading-related files mention persistence requirements # Search for persistence-related configurations in trading module rg -i "persist|storage" --type ts --glob "suite-native/module-trading/**"Length of output: 75
Action Required: Confirm Trading State Persistence
The trading reducer appears to be prepared correctly using
tradingSlice.prepareReducer(extraDependencies). However, unlike other reducers (e.g., those managing accounts or transactions), it lacks any explicit persistence configuration. Our search in thesuite-native/module-tradingdirectory did not reveal any persistence-related settings. Please verify manually whether the omission of persistence for the trading state is intentional or if it requires alignment with the persistence patterns in other parts of the codebase.
There was a problem hiding this comment.
LGTM. I have just one note. Reducer is a logic that is IMO most simple to test and I even like to use it as good example where TDD can be applied, because responsibility and expected results are clear.
I usually start like:
it('should have correct initial state', () => {
const state = myReducer(undefined, {action: 'unknown'});
expect(state).toEqual(expectedInitialState);
});
describe('myAction1', () => {
it('should set X', () => {
const state = myReucer(undefined, myAction1());
expect(state).toEqual(expect.objectContaining({ x: true }));
});
describe('selectMyX', /* ...*/);
});Would it be possible to add some basic test coverage please?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/module-trading/src/tradingSlice.ts (1)
46-47: Improve comment clarity.The current comment could be more descriptive about the fallback behavior.
- // In case that this reducer does not match the action, try to handle it by suite-common tradingReducer. + // Fallback to suite-common tradingReducer for unhandled actions to maintain compatibility with common trading functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-trading/src/tradingSlice.ts(1 hunks)suite-native/state/src/reducers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/state/src/reducers.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (6)
suite-native/module-trading/src/tradingSlice.ts (6)
1-12: LGTM! Well-organized imports with proper scoping.The imports are properly organized, with clear separation between external dependencies and internal modules.
13-25: LGTM! Well-structured type definitions.The interfaces and types are well-defined with proper extension of common types, providing good type safety and clear state structure.
27-30: LGTM! Proper state initialization.The initial state is correctly defined, preserving common state while extending with new fields.
32-42: LGTM! Well-structured Redux slice.The slice is well-implemented with proper state mutations using Redux Toolkit's Immer integration.
53-53: LGTM! Proper action export.The action creator is correctly exported following Redux best practices.
59-61: Consider renaming selector and simplifying implementation.Based on the past review comments, selectors should not be prefixed with "select" unless they are actual selectors. Additionally, the implementation could be simplified.
-export const selectBuySelectedReceiveAccount = (state: TradeRootState) => - selectTradingBuy(state).selectedReceiveAccount; +export const buyReceiveAccount = (state: TradeRootState) => selectTradingBuy(state).selectedReceiveAccount;
|
🚀 Expo preview is ready!
|
trezor-ci
force-pushed
the
feat/native/introduce-trade-reducer
branch
from
0d0dce5 to
8e6871f
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
/rebase |
trezor-ci
force-pushed
the
feat/native/introduce-trade-reducer
branch
from
f392bd9 to
15eb648
Compare
vytick
force-pushed
the
feat/native/introduce-trade-reducer
branch
from
15eb648 to
0ed7563
Compare
Adds trading slice with tradingReducer to mobile app